Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ComputeTriangleAreas, GetNonManifoldEdges and RemoveNonManifoldEdges in t::geometry::TriangleMesh #6657

Merged
merged 5 commits into from
Jun 15, 2024

Conversation

nsaiapova
Copy link
Contributor

Implement a few methods which are available with the geometry::TriangleMesh API.

Type

  • Bug fix (non-breaking change which fixes an issue): Fixes #
  • New feature (non-breaking change which adds functionality). Resolves #
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) Resolves #

Motivation and Context

Checklist:

  • I have run python util/check_style.py --apply to apply Open3D code style
    to my code.
  • This PR changes Open3D behavior or adds new functionality.
    • Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is
      updated accordingly.
    • I have added or updated C++ and / or Python unit tests OR included test
      results
      (e.g. screenshots or numbers) here.
  • I will follow up and update the code if CI fails.
  • For fork PRs, I have selected Allow edits from maintainers.

Description

In this PR three methods for t::geometry::TriangleMesh are implemented.

t::geometry::TriangleMesh::ComputeTriangleAreas
Split the logic from ComputeSurfaceArea into a helper static function
and introduce a new method which computes triangle areas and writes the
resulting tensor into attributes of the mesh.
IIUC, in geometry::TriangleMesh the corresponding function is GetSurfaceArea(std::vector &areas), where the areas are filled with the triangle areas.

t::geometry::TriangleMesh::GetNonManifoldEdges mimics the logic of the legacy method.

t::geometry::TriangleMesh::RemoveNonManifoldEdges follows the logic of
the legacy method but there are a few differences:

  • the main difference is that the outer while-loop is removed. I don't
    see how after the first iteration any edge can have more than 2
    adjucent triangles, which makes the further iterations unnecessary.
  • I count triangles with non-negative areas immediately and do not rely
    on the total number of adjacent triangles (which would also include
    triangles marked for removal).
  • To choose a triangle with the minimal area out of the existing
    adjacent triangles I use a heap structure.

Copy link

update-docs bot commented Feb 15, 2024

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

Copy link
Member

@ssheorey ssheorey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nsaiapova! Some minor comments. Feel free to ignore comments labeled with nit. We can chat about the algorithm....

python/test/t/geometry/test_trianglemesh.py Outdated Show resolved Hide resolved
@@ -1212,5 +1212,84 @@ TEST_P(TriangleMeshPermuteDevices, SelectByIndex) {
box_untouched.GetTriangleIndices()));
}

TEST_P(TriangleMeshPermuteDevices, RemoveNonManifoldEdges) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this - looks like we didn't have a test for this in legacy geometry.

python/test/t/geometry/test_trianglemesh.py Outdated Show resolved Hide resolved
cpp/open3d/t/geometry/TriangleMesh.h Outdated Show resolved Hide resolved
cpp/open3d/t/geometry/TriangleMesh.h Outdated Show resolved Hide resolved
cpp/open3d/t/geometry/TriangleMesh.cpp Show resolved Hide resolved
cpp/open3d/t/geometry/TriangleMesh.cpp Outdated Show resolved Hide resolved
cpp/open3d/t/geometry/TriangleMesh.cpp Outdated Show resolved Hide resolved
cpp/open3d/t/geometry/TriangleMesh.cpp Outdated Show resolved Hide resolved
cpp/open3d/t/geometry/TriangleMesh.cpp Outdated Show resolved Hide resolved
@nsaiapova nsaiapova force-pushed the wip/remove-non-manifold-edges branch from 3b8f865 to 20782b4 Compare April 16, 2024 10:52
@ssheorey
Copy link
Member

Hi @nsaiapova can you take a look at the failing CI checks?
Looks like we are almost ready to merge after that.

@ssheorey ssheorey added this to the v0.19 milestone Apr 29, 2024
@nsaiapova nsaiapova force-pushed the wip/remove-non-manifold-edges branch from 20782b4 to aa7a3ce Compare April 30, 2024 13:35
Split the logic from ComputeSurfaceArea into a helper static function
and introduce a new method which computes triangle areas and writes the
resulting tensor into attributes of the mesh.
@nsaiapova nsaiapova force-pushed the wip/remove-non-manifold-edges branch from aa7a3ce to 110aebd Compare April 30, 2024 14:12
... and t::geometry::TriangleMesh::GetNonManifoldEdges methods.
The methods are defined in geometry::TriangleMesh API.

t::geometry::TriangleMesh::GetNonManifoldEdges mimics the logic of the
legacy method.

t::geometry::TriangleMesh::RemoveNonManifoldEdges follows the logic of
the legacy method but there are a few differences:
* the main difference is that the outer while-loop is removed. I don't
  see how after the first iteration any edge can have more than 2
  adjacent triangles, which makes the further iterations unnecessary.
* I count triangles with non-negative areas immediately and do not rely
  on the total number of adjacent triangles (which would also include
  triangles marked for removal).
* To choose a triangle with the minimal area out of the existing
  adjacent triangles I use a heap structure.
@nsaiapova nsaiapova force-pushed the wip/remove-non-manifold-edges branch from 110aebd to e25faf0 Compare April 30, 2024 14:24
@ssheorey
Copy link
Member

Update unit tests to unordered comparison, since order of output is platform dependent.

Copy link
Member

@ssheorey ssheorey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nsaiapova ! Looks good!

…pty areas property if there are no triangles.
@ssheorey ssheorey force-pushed the wip/remove-non-manifold-edges branch from 6f2dd07 to b008cb7 Compare June 15, 2024 05:15
@ssheorey ssheorey force-pushed the wip/remove-non-manifold-edges branch from 74230d4 to cf99e70 Compare June 15, 2024 16:05
@ssheorey ssheorey merged commit ad17a26 into isl-org:main Jun 15, 2024
31 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants